Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUACAMOLE-1006: Provide implementations for List properties. #490

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

necouchman
Copy link
Contributor

Provides some basic implementations for String and URI list properties in guacamole-ext, in anticipation of those being required for things like LDAP and RADIUS server redundancy.

Also, note that I switched over to semicolon delimiting for these - that's worth some discussion, but my rationale for that is that some URIs - LDAP ones, for instance - rely on the ability to use commas to separate LDAP DNs within the URI, so, one way or the other we need to address that.

@mike-jumper
Copy link
Contributor

What do you think about enhancing Environment such that it inherently supports retrieving an ordered Collection of property values?

It could then be up to the GuacamoleProperty implementation to decide how to represent multiple values (if at all).

@necouchman
Copy link
Contributor Author

Sounds good - I will take a look.

@necouchman
Copy link
Contributor Author

@mike-jumper Okay, I've started down path that I think is approximately what you're suggesting, while trying to avoid breaking everything in the process. So, instead of making getProperty() and/or getRequiredProperty() return a collection, I added a new set of methods, getPropertyCollection() and getRequiredPropertyCollection() that are intended to parse into a Collection.

This would rely on each of the properties to implement a new getValueCollection() method, which I've provided a default implementation for in GuacamoleProperty that basically says "not yet implemented."

Let me know if this looks good, sane, reasonable, etc., or if there's a better solution? I could break everything and make getValue(), getProperty(), and getRequiredProperty() return collections instead of individual values, if that's preferable?

@necouchman necouchman force-pushed the jira/1006 branch 2 times, most recently from e264889 to 98ef4ae Compare July 14, 2024 17:52
@necouchman
Copy link
Contributor Author

@mike-jumper Okay, I've re-worked the implementation, here, providing support within the existing property classes for parsing and retrieving lists of values.

@necouchman necouchman requested a review from mike-jumper July 14, 2024 17:54
@mike-jumper mike-jumper changed the base branch from main to staging/1.6.0 July 31, 2024 21:35
Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default implementations for new functions were added to the GuacamoleProperty interface, but I think they're missing from Environment.

Otherwise, LGTM!

@necouchman
Copy link
Contributor Author

necouchman commented Aug 30, 2024

default implementations for new functions were added to the GuacamoleProperty interface, but I think they're missing from Environment.

Otherwise, LGTM!

Okay, I think I managed to get these default methods implemented correctly in the Environment interface, but for some reason it was a bit of a mind-bending exercise on the order of the spoon in The Matrix.

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slick approach - I see what you mean about the spoon.

@mike-jumper mike-jumper merged commit 0301a11 into apache:staging/1.6.0 Aug 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants